Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support of upsert for Postgres and MySQL #987

Merged
merged 1 commit into from
Apr 12, 2018
Merged

Conversation

mosyp
Copy link
Collaborator

@mosyp mosyp commented Nov 30, 2017

Fixes #16
Fixes #595

Taking into account experience from #595 I've came up with an another implementation of upsert feature for postgres and mysql

Checklist

  • Unit test all changes
  • Update README.md if applicable
  • Add [WIP] to the pull request title if it's work in progress
  • Squash commits that aren't meaningful changes
  • Run sbt scalariformFormat test:scalariformFormat to make sure that the source files are formatted

@getquill/maintainers

@mosyp mosyp changed the title [WIP] Ass support of upsert for Postgres and MySQL [WIP] Add support of upsert for Postgres and MySQL Nov 30, 2017
@mosyp mosyp force-pushed the upsert-v2 branch 2 times, most recently from 613f05a to 956c27e Compare December 9, 2017 12:47
@mosyp mosyp force-pushed the upsert-v2 branch 4 times, most recently from a2e4f6d to aee5a50 Compare January 11, 2018 18:50
@mosyp mosyp force-pushed the upsert-v2 branch 2 times, most recently from aa559a1 to f1926ee Compare January 13, 2018 11:28
@Nickersoft
Copy link

Any update on this?

@mosyp
Copy link
Collaborator Author

mosyp commented Jan 30, 2018

@Nickersoft Hello, I'm working on it when I have time. I have plans to ship it with next 2.4.0 release

@Nickersoft
Copy link

@mentegy Cool! Thanks for the update :)

@mosyp mosyp force-pushed the upsert-v2 branch 4 times, most recently from ce117c3 to c0682ae Compare February 7, 2018 18:16
@mosyp mosyp force-pushed the upsert-v2 branch 3 times, most recently from 29f41c2 to 3853c18 Compare April 4, 2018 08:42
@mosyp mosyp changed the title [WIP] Add support of upsert for Postgres and MySQL Add support of upsert for Postgres and MySQL Apr 6, 2018
@mosyp
Copy link
Collaborator Author

mosyp commented Apr 6, 2018

@getquill/maintainers It's ready for review. Please refer information in readme for more details

@@ -92,6 +92,8 @@ case class If(condition: Ast, `then`: Ast, `else`: Ast) extends Ast

case class Assignment(alias: Ident, property: Ast, value: Ast) extends Ast

case class Excluded(alias: Ident) extends Ast
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you give a more descriptive name to these new AST classes? Or maybe nest them within an object that has a name that clarifies their purpose.

@@ -129,6 +131,16 @@ case class Returning(action: Ast, alias: Ident, property: Ast) extends Action

case class Foreach(query: Ast, alias: Ident, body: Ast) extends Action

case class Conflict(insert: Ast, target: Conflict.Target, action: Conflict.Action) extends Action
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think OnConflict would be a better name

@fwbrasil
Copy link
Collaborator

fwbrasil commented Apr 9, 2018

besides the naming, everything else LGTM

@fwbrasil
Copy link
Collaborator

fwbrasil commented Apr 9, 2018

@mentegy the compilation is failing:

[error] /app/quill-core/src/test/scala/io/getquill/context/mirror/MirrorIdiomSpec.scala:457:19: object Conflict is not a member of package io.getquill.ast
[error]         stmt"${(i.onConflictIgnore.ast: Ast).token}" mustEqual
[error]                   ^
[error] /app/quill-core/src/test/scala/io/getquill/context/mirror/MirrorIdiomSpec.scala:461:35: object Conflict is not a member of package io.getquill.ast
[error]         stmt"${(i.onConflictIgnore(_.i, _.s).ast: Ast).token}" mustEqual
[error]                                   ^
[error] /app/quill-core/src/test/scala/io/getquill/context/mirror/MirrorIdiomSpec.scala:465:35: object Conflict is not a member of package io.getquill.ast
[error]         stmt"${(i.onConflictUpdate((t, e) => t.s -> e.s, (t, e) => t.i -> (t.i + 1)).ast: Ast).token}" mustEqual
[error]                                   ^
[error] /app/quill-core/src/test/scala/io/getquill/context/mirror/MirrorIdiomSpec.scala:469:40: object Conflict is not a member of package io.getquill.ast
[error]         stmt"${(i.onConflictUpdate(_.i)((t, e) => t.s -> e.s).ast: Ast).token}" mustEqual
[error]                                        ^
[error] /app/quill-core/src/test/scala/io/getquill/context/mirror/MirrorIdiomSpec.scala:457:36: value ast is not a member of io.getquill.testContext.Insert[io.getquill.testContext.TestEntity]
[error]         stmt"${(i.onConflictIgnore.ast: Ast).token}" mustEqual
[error]                                    ^
[error] /app/quill-core/src/test/scala/io/getquill/context/mirror/MirrorIdiomSpec.scala:461:46: value ast is not a member of io.getquill.testContext.Insert[io.getquill.testContext.TestEntity]
[error]         stmt"${(i.onConflictIgnore(_.i, _.s).ast: Ast).token}" mustEqual
[error]                                              ^
[error] /app/quill-core/src/test/scala/io/getquill/context/mirror/MirrorIdiomSpec.scala:465:86: value ast is not a member of io.getquill.testContext.Insert[io.getquill.testContext.TestEntity]
[error]         stmt"${(i.onConflictUpdate((t, e) => t.s -> e.s, (t, e) => t.i -> (t.i + 1)).ast: Ast).token}" mustEqual
[error]                                                                                      ^
[error] /app/quill-core/src/test/scala/io/getquill/context/mirror/MirrorIdiomSpec.scala:469:63: value ast is not a member of io.getquill.testContext.Insert[io.getquill.testContext.TestEntity]
[error]         stmt"${(i.onConflictUpdate(_.i)((t, e) => t.s -> e.s).ast: Ast).token}" mustEqual

@hntd187
Copy link

hntd187 commented Apr 9, 2018

Does any of this borrow from the pull req I made a lifetime ago? If it's a continuation of that can I close the other since @mentegy has seemed to pick this up?

@mosyp
Copy link
Collaborator Author

mosyp commented Apr 9, 2018

@fwbrasil I'll fix it tomorrow once will get access to my laptop.
@hntd187 I was thinking of doing that once this one will be merged

@mosyp
Copy link
Collaborator Author

mosyp commented Apr 10, 2018

@getquill/maintainers It's all green now!
(except coverage, however not covered parts are not PR changes)

@mosyp
Copy link
Collaborator Author

mosyp commented Apr 12, 2018

@fwbrasil Build is passing and PR is updated regarding your notes. This PR blocks our next release

@fwbrasil
Copy link
Collaborator

@mentegy I think you missed my comments about naming

@mosyp
Copy link
Collaborator Author

mosyp commented Apr 12, 2018

@fwbrasil oops, I must have pushed wrong version when syncing with upstream. Now your notes are applied

@fwbrasil
Copy link
Collaborator

This is exciting! Users have been asking for this feature for so long

@fwbrasil fwbrasil merged commit c5760eb into zio:master Apr 12, 2018
@dsounded
Copy link

@mentegy Awesome )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants